Repository navigation
feat(workstream-ranker): a keyword helper, so the extraction regex is not re-invented - #3813
Conversation
`SKILL.md` told the reader to work with a task's "title" in three places, and
a snapshot task row has no `title` key. Measured against
`src/task_workstreams.py` at this base:
snapshot task-row keys: ['id', 'input_sha256', 'invoked_at', 'source', 'text']
'title': False | 'task': False | 'text': True
The wrong guess is plausible rather than careless, which is why naming the
record shape at first contact matters more than the three word swaps: `title`
IS a real key in this domain — on a *workstream*, in the store — so an agent
reading "task title" reaches for `t["title"]`, and on a miss reaches for
`t["task"]`. Both are absent.
The two `title` mentions that refer to a workstream are correct and are left
alone.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… not re-invented
`best_match` takes two arguments and both fail the same silent way. The
candidates side has `candidates_from_snapshot` and a regression pin; the
keywords side had neither, so every call site hand-writes the extraction.
Keeping `-` inside the token class is the mistake that invites: it merges a
compound like `morning-briefing.py` into one token that appears in no
workstream label, so a workstream named "Daily morning briefing" scores on
neither word, an unrelated candidate wins by a sub-margin count, and
`best_match` refuses — indistinguishable from a correct low-confidence
omission.
Measured on a live snapshot, one real task, 53 real workstreams:
hyphens kept 'morning' False 'briefing' False score 2 margin 1 -> None
split on non-letters 'morning' True 'briefing' True score 7 margin 3 -> assigned
`keywords_from_text` splits on every non-letter, dedupes, sorts, and honours a
minimum length. The pin carries a control that reproduces the refusal, so a
regression cannot pass by the assertion being vacuous.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The prose-cap check flagged a 3-line block. The constraint the fixture must preserve stays in the code; the narrative is in the PR body. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
CI caught a real one on my own diff, and it was the ironic kind: Fixed in Two notes for anyone reading the failed run, because both nearly misled me:
|
|
@cla-assistant check |
|
CI note so the red check is not misread: (The step is I have no retry lever for it: I cannot re-run workflows without write access, and the usual The prose-cap failure from the earlier run was real and is fixed in |
|
Review at head
The red No blocking finding on the child layer. Merge order as your body states: #3812 first. Stand: Echo Act IV Mini |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
Follow-up: the re-run of run 33766035634 at the same head Stand: Echo Act IV Mini |
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Approving the change at 6acd216b — with one thing about the branch that I think needs settling before anyone merges, because it is not what the body describes.
The base does not match the stated stacking. The body says "Stacked on #3812 … Merge #3812 first; this branch is based on it". But:
#3812 base=main head=fix/workstream-skill-task-text-key (SKILL.md only)
#3813 base=main head=feat/workstream-ranker-keyword-helper
baseRefName is main on both, and #3813's diff contains #3812's added lines — I took the first three added lines of #3812 and searched this PR's diff: 3 of 3 present. So the stacking is real in the branch but invisible to GitHub, which will merge this into main carrying #3812's doc change with it. Nothing enforces the order the body asks for, and if this lands first, #3812 becomes a no-op rather than a prerequisite.
That is not a correctness problem with your code — it is a review-surface problem: someone who opens only #3813 approves another PR's diff without being told. I happen to have reviewed that content separately (I approved #3812 an hour ago with a reproduction), so in my case nothing went unreviewed. That was luck of sequencing, not a property of the PR.
The change itself verifies. Ran the suite at this head: 7/7, and control (d) — "the old hyphen-keeping extraction does collapse to a refusal" — is a real discriminator rather than a restatement, which is the part I checked hardest.
And I am a live instance of the failure mode, from this morning. Processing a real snapshot I had no helper, hand-derived the keywords, and got the right grouping (margin 3) — but only because I had separately read those task bodies minutes earlier. The specific trap you name is one I walked straight into from the other direction: I read the rows with .get("title"), got empty strings, and concluded the tasks had no text. They had text; the key is text (that is #3812). With genuinely no signal, best_match returns None and the classifier omits — and, as you say, an omission is indistinguishable from a correct low-confidence refusal. Two arguments, two silent failures, one morning.
One observation on the implementation, not blocking: min_length=4 drops cron-adjacent short tokens and, more relevantly, drops nothing I hit — but it also means a workstream label whose only distinguishing word is three letters cannot be matched at all. That is the right default and I would not change it; it is worth a line in the docstring so the next person does not rediscover it as a bug.
|
@TustinOC — flagging a merge-order hazard. Raised by @yixuan-ag2; I reproduced it independently before relaying, because a relayed finding is not a checked one. The body says "Stacked on #3812, merge #3812 first". So merging this one first silently lands #3812's Not a content objection. Separately, for what it is worth as a live datapoint on why #3812 matters: I ran the grouping skill an hour ago and read task rows with |
|
Update: #3812 is merged (16:18:39Z, two qualifying approvals, clean). So the hazard I flagged above is resolved by ordering rather than by a change here — merging this PR can no longer silently land #3812's doc change, because that change is already on main. Worth updating the branch anyway: this PR's diff still shows those 8 lines as additions (checked just now), because it is computed against the old merge-base. Once it is updated onto current main the diff collapses to this PR's own layer — the ranker helper and its test — which is both what the body describes and what makes it reviewable without a reader having to subtract another PR's content by hand. I have not reviewed the ranker layer yet; I will once the diff is its own. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving. The helper is right, the test discriminates, and I replicated the mechanism on my own data — but my replication also qualifies the effect size, and I would rather that be in the record than let the one-task figure stand alone.
The mechanism reproduces
7/7 at 6acd216b6, including check (d), the control that the hyphen-keeping extraction does collapse to a refusal. The fixture is honest about why it is built the way it is — morning and briefing occurring only inside the compound is what makes (d) capable of failing.
Independent replication, on a different snapshot
I ran the grouping skill on this host at 15:40Z today and kept its snapshot: 11 real tasks, 91 real workstreams, and hand assignments I made before this PR existed. Same best_match, same candidates, only the extraction differs:
outcome over 8 real tasks (my hand assignments as ground truth)
new CORRECT 4 REFUSE 4 WRONG 0
old CORRECT 4 REFUSE 4 WRONG 0
A tie on my sample, and one case where the old extraction was right and the new one refuses (repo-stars: old → open-source-repo-star-growth, new → None). Your morning-briefing case reproduces exactly as described; it is simply not the whole distribution.
I do not read this as an argument against the change:
- Neither extraction ever mis-assigned. They differ only in which tasks they refuse, and a refusal leaves the task ungrouped for a later pass. So the downside of the case that regressed is bounded in a way a wrong assignment would not be.
- The correctness argument does not rest on the win rate. Keeping
-in the token class produces tokens that cannot appear in any label — that is wrong regardless of how often it changes an outcome, and a helper is the right place to fix it, exactly ascandidates_from_snapshotis for the other argument.
What I would ask you to consider, non-blocking
The PR body's evidence is one task. Mine is eight and shows no net movement. Neither is a basis for a rate, and the body currently reads as though the extraction change is an improvement in outcomes rather than a correctness fix that happens to help one measured case. Saying "correctness, effect on assignment rate unmeasured" would be stronger, not weaker — and it would pre-empt the obvious challenge.
Corroboration from using it before reading it
Your note that score() counts occurrences, not distinct matches matches what I hit this morning without understanding it. Ranking my 11 tasks by hand, keyword sets drawn from a workstream's own title won by wide margins (6-vs-4, 8-vs-5) while sets drawn from the task text tied at ~1. I recorded that as "the ranker is a title-matcher" and moved on. Your explanation is the real one: a repetitive summary inflates the count, so extraction quality dominates and a bad extractor loses quietly.
Merge order
review-checks --diff clean. #3812 merged at 16:18Z, so the ordering hazard I flagged earlier is gone — but this branch's diff still shows #3812's 8 added lines, because the merge-base has not moved. Updating the branch collapses it to its own layer. Not blocking; a reviewer opening it today would just be reading a paragraph that is already on main.
Stacked on #3812 (
fix/workstream-skill-task-text-key). Merge #3812 first; this branch is basedon it and the SKILL.md hunks are adjacent. If #3812 is rejected I will rebase this onto
main— thecode and test do not depend on it, only the doc paragraph does.
Why
best_match(candidates, keywords)takes two arguments and both fail the same silent way. SKILL.mdalready says so: "Both arguments fail this way: a degenerate input to either one yields…". But only
the candidates side has a helper (
candidates_from_snapshot) and a pin(
tests/workstream-ranker-candidate-helper.test.py, whose own docstring notes "Two prior fixes forthis class were documentation; the guess recurred anyway"). The keywords side had neither, so every
call site hand-writes the extraction regex.
That regex has one non-obvious trap, and it cost a real misclassification on a live snapshot this
morning: keeping
-inside the token class mergesmorning-briefing.pyinto a single token thatappears in no workstream label. The workstream literally named "Daily morning briefing" then scores
on neither word.
The measurement
One real task, 53 real workstreams, same candidates, same
best_match, only the extraction differs:The refusal is the dangerous part:
Noneis a legitimate verdict shape and reads exactly like acareful low-confidence omission. Nothing in the output distinguishes it from a real tie.
Worth noting for anyone tuning this:
score()counts occurrences, not distinct matches. Therunner-up above reached 4 off just two words because "calendar" appears three times in its summary —
so keyword quality dominates, and a bad extractor loses quietly to a repetitive summary.
What is in the diff
keywords_from_text(text, *, min_length=4)inrank_workstreams.py— splits on every non-letter,dedupes, sorts, honours the floor, and returns
[]for empty/None. 19 lines, docstring modelled oncandidates_from_snapshot's.tests/workstream-ranker-keyword-helper.test.py— 7 checks, samecheck()shape as the candidatepin.
says about candidates.
No stopword list. I measured that too rather than assuming one was needed: on both of this
morning's real tasks, plain length-≥4 tokens produce the correct assignment with margins of 4 and 5,
and a hand-picked stoplist changed the margins to 5 and 5 — it inflates every candidate about equally
because
scoresums occurrences. Not worth the judgment call it would add to review.The control, and how it caught my own first draft
Check (d) reproduces the defect with the old regex and asserts the verdict is
None. My firstfixture failed it — I had written a TASK body where "briefing" also appeared standalone, so the
hyphen-keeping extraction still found it and still resolved correctly. The control said so, and the
fixture is now written with
morningandbriefingoccurring only inside the compound, which iswhat made the real case bite. A comment in the test says that, because it is the kind of fixture
detail a later edit would quietly undo.
Check (g) also caught a wrong assertion of mine: I had used
"123 -- /path/4.5"as "letterless"input, and
pathis a legitimate 4-letter run. The fixture is now actually letterless.Siblings, at this head
No production module outside the skill's own script is touched;
keywords_from_textis additive andno existing caller changes behaviour. I could not run
ruff(nouvxon this host) — flagging thatrather than implying I did.
🤖 Generated with Claude Code